support absolute root path for serveStatic #78
Conversation
| /** | ||
| * wrapper for getFilePath, with absolute root path | ||
| */ | ||
| export function getFilePathforAbsRoot(options: { |
There was a problem hiding this comment.
I think that getFilePathForAbsRoot would be a better name.
There was a problem hiding this comment.
I agree.
I am considering a better name but have not yet been able to come up with one.
The current getFilePath function has the following features:
- the relative path to the parent should be undefind (
../a=> undefined) - it normalizes the path for KV (considering the relative path to KV root
./hoge/fuga=>hoge/fuga,/hoo=>hoo) - it appends defaultDocument (
/hoo=>/hoo/index.html)
I think that getFilePath has two roles:
features 1 and 2 are for KV, and the rest are for general use to serve something. Is that correct?
Relative to the parent or absolute path doesn't work because of the features 1 and 2.
I've tested that serveStatic in Bun and Deno also doesn't work.
I believe it would be better to prepare another function specifically for KV that utilizes getFilePath and implements features for KV as follows:
function getFilePathForKV(options :FilePathOptions) { // or more better function name
if (/(?:^|\/)\.\.(?:$|\/)/.test(options.filename)) return
const path = getFilePath(options)
// ./assets/foo.html => assets/foo.html
return path.replace(/^\.?\//, '')
}What are your thoughts on this? If you agree with this opinion, I will create a pull request for it.
There was a problem hiding this comment.
These are for both KV and other file system-based runtimes.
'1' is necessary to avoid issues such as directory traversal. In older versions of Bun, the URL path was not normalized, so there was a possibility that the URL path included .., which could lead to unexpected behavior. I think we can handle it in a different place, such as serve-static for Bun, but currently, we are handling it in getFilePath().
'2' is to normalize the path to find the file. The file path emitted from getFilePath() will be used as shown in the link:
In KV as well, /foo will fall back to /foo/index.html. So, this is necessary for KV.
|
HI @miyamonz ! Thank you for the PR. This is a good feature. I've leaved some comments. Please check them! |
|
I've reconsidered this feature. Allowing absolute paths could potentially lead to a traversal attack vulnerability. If we decide to proceed, we need to address this risk seriously. I would like to hear opinions about this potential security issue. |
|
I'm not sure I see the concern. It's common enough to offer absolute path as root, and it's done by disallowing "/../" in the path. For example, this is what https://github.com/pillarjs/send/blob/b69cbb3dc4c09c37917d08a4c13fcd1bac97ade5/index.js#L63 Could you elaborate where a threat could come from? |
|
In Hono, we also do not allow https://github.com/honojs/hono/blob/main/src/utils/filepath.ts#L9 |
|
That's reassuring. What do you think is next to progress this PR? |
|
Any progress on this PR? It's very useful in some cases. |
Workaround: honojs/hono#3108 (comment) |
|
I've just stumbled upon this issue. I think one way to move this forward is to add a field like |
|
Why not just introduce another param?
And then merge them at some point when hono has a major. |
|
Should this be implemented in Then just pass |
|
Hey! I'll work on this matter from now. |
Currently, serveStatic's options.root doesn't support absolute paths.
It appears that the getFilePath utility function isn't designed for this case.
I've created a small wrapper to address this issue.